Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: add test for /debug/sub-optimal-plan HTTP API #14302

Merged
merged 6 commits into from
Jan 6, 2020

Conversation

Deardrops
Copy link
Contributor

@Deardrops Deardrops commented Dec 31, 2019

What problem does this PR solve?

Improving unit test coverage of server package.

What is changed and how it works?

add test for /debug/sub-optimal-plan HTTP API, This API added by #10717

Before this PR:

ok  	github.com/pingcap/tidb/server	25.799s	coverage: 65.1% of statements

After this PR:

ok  	github.com/pingcap/tidb/server	30.378s	coverage: 68.4% of statements

Check List

Tests

  • Integration test

Code changes

  • None

Side effects

  • Possible performance regression

The test time of server package increased from 25s to 30s.

Related changes

  • None

Release note

  • None

@Deardrops
Copy link
Contributor Author

/run-all-tests

@Deardrops
Copy link
Contributor Author

/run-unit-test

@@ -255,14 +273,8 @@ func (sh *sqlInfoFetcher) getExplainAnalyze(ctx context.Context, sql string, res
resultChan <- &explainAnalyzeResult{rows: rows}
}

func (sh *sqlInfoFetcher) catchCPUProfile(ctx context.Context, sec int, zw *zip.Writer, errChan chan<- error) {
// dump profile
fw, err := zw.Create("profile")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call zw.Create("profile") in dumpCPUProfile() instead of catchCPUProfile(),
For avoiding data race with

fw, err := zw.Create("explain_analyze.txt")

c.Assert(len(b), Greater, 0)
c.Assert(resp.Body.Close(), IsNil)

resp, err = http.PostForm("http://127.0.0.1:10090/debug/sub-optimal-plan?pprof_time=5&timeout=0", urlValues)
c.Assert(err, IsNil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add more test for pprof_time & timeout & urlValues . like

  1. only use single pprof_time or single timeout
  2. set wrong 'curDB'

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AilinKid AilinKid added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 3, 2020
@reafans
Copy link
Contributor

reafans commented Jan 3, 2020

/run-all-tests

@reafans
Copy link
Contributor

reafans commented Jan 3, 2020

Rest LGTM

@Deardrops
Copy link
Contributor Author

/run-all-tests

@Deardrops
Copy link
Contributor Author

/run-unit-test

@Deardrops Deardrops added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 3, 2020
Copy link
Contributor

@reafans reafans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sre-bot
Copy link
Contributor

sre-bot commented Jan 5, 2020

@AilinKid, @reafans, @winoros, PTAL.

@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. and removed status/PTAL labels Jan 6, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 6, 2020

Your auto merge job has been accepted, waiting for 14200, 14035, 14275, 14229, 14357

@sre-bot
Copy link
Contributor

sre-bot commented Jan 6, 2020

/run-all-tests

@sre-bot sre-bot merged commit 297e453 into pingcap:master Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants